-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add github actions CI and remove travis CI #89
Conversation
Note that running tests on macOS require root, since we access |
In this case the tests can't even be built. Look at the errors. My guess is that |
Yes either rebuild bindings with new bindgen or pick it up from #87 where I had also updated pfvar.h with the most recent. |
05fa368
to
44c2eef
Compare
b481d49
to
8e9553f
Compare
8ae46dc
to
2578f19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 7 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern)
.github/workflows/build-and-test.yml
line 37 at r4 (raw file):
# Since the tests modify global state (the system firewall) they cannot run in parallel. # The integration tests must run as root since they write firewall state (/dev/pf) run: sudo cargo test -- --test-threads=1
The output of the action seems to indicate that this command causes a rebuild, which will then be done with root access
.github/workflows/cargo-audit.yml
line 35 at r4 (raw file):
# Ignored audit issues. This list should be kept short, and effort should be # put into removing items from the list. ignore:
The ignore section is empty, should it still be there?
.github/workflows/formatting.yml
line 5 at r4 (raw file):
on: pull_request: paths:
Should rustfmt.toml
not be included here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)
.github/workflows/cargo-audit.yml
line 35 at r4 (raw file):
Previously, Serock3 (Sebastian Holmin) wrote…
The ignore section is empty, should it still be there?
I thought it was better to keep it. Since it makes it easier to add when needed. Most projects will oscillate between zero and ~1-2 ignored issues. And if they every time they go from zero to non-zero have to add the boilerplate including the docs, that's more churn. I figured it's nicer to keep the docs and keyword even when empty. Do you agree?
.github/workflows/formatting.yml
line 5 at r4 (raw file):
Previously, Serock3 (Sebastian Holmin) wrote…
Should
rustfmt.toml
not be included here?
I was hoping you would not see that one. Because I aim to remove that settings file but I wanted to keep it to a separate PR to not scope creep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)
.github/workflows/formatting.yml
line 5 at r4 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I was hoping you would not see that one. Because I aim to remove that settings file but I wanted to keep it to a separate PR to not scope creep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)
.github/workflows/cargo-audit.yml
line 35 at r4 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I thought it was better to keep it. Since it makes it easier to add when needed. Most projects will oscillate between zero and ~1-2 ignored issues. And if they every time they go from zero to non-zero have to add the boilerplate including the docs, that's more churn. I figured it's nicer to keep the docs and keyword even when empty. Do you agree?
Alright
.github/workflows/formatting.yml
line 5 at r4 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I was hoping you would not see that one. Because I aim to remove that settings file but I wanted to keep it to a separate PR to not scope creep.
Haha I see, let's ignore it for now then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)
.github/workflows/build-and-test.yml
line 37 at r4 (raw file):
Previously, Serock3 (Sebastian Holmin) wrote…
The output of the action seems to indicate that this command causes a rebuild, which will then be done with root access
Yes. But as I wrote on slack (repeating just for documentation):
- Building the entire thing takes ~10 seconds. So optimizing is hardly worth it
- There are no secrets on this build machine/container, so it does not matter if its compromized
- The container has passwordless sudo anyway. So it would be trivial to compromise even if we did not use sudo ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
The CI for this crate has not been running for a looong time. We have not used Travis for years.
This PR:
I wish we could also run
cargo clippy
. Hopefully we can in the future. But currently clippy complains about so much that I leave it as a separate exercise.This change is